Skip to content

std: lazily allocate the main thread handle #132654

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 4 commits into from
Jan 15, 2025
Merged

std: lazily allocate the main thread handle #132654

merged 4 commits into from
Jan 15, 2025

Conversation

joboet
Copy link
Member

@joboet joboet commented Nov 5, 2024 •
edited
Loading

#123550 eliminated the allocation of the main thread handle, but at the cost of greatly increased complexity. This PR proposes another approach: Instead of creating the main thread handle itself, the runtime simply remembers the thread ID of the main thread. The main thread handle is then only allocated when it is used, using the same lazy-initialization mechanism as for non-runtime use of thread::current, and the name method uses the thread ID to identify the main thread handle and return the correct name ("main") for it.

Thereby, we also allow accessing thread::current before main: as the runtime no longer tries to install its own handle, this will no longer trigger an abort. Rather, the name returned from name will only be "main" after the runtime initialization code has run, but I think that is acceptable.

This new approach also requires some changes to the signal handling code, as calling thread::current would now allocate when called on the main thread, which is not acceptable. I fixed this by adding a new function (with_current_name) that performs all the naming logic without allocation or without initializing the thread ID (which could allocate on some platforms).

Reverts #123550, CC @GnomedDev

@rustbot
Copy link
Collaborator

rustbot commented Nov 5, 2024

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Nov 5, 2024
@joboet joboet changed the title Lazy main std: lazily allocate the main thread handle Nov 5, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@workingjubilee
Copy link
Member

workingjubilee commented Nov 5, 2024 •
edited
Loading

I've also moved the Thread code into its own module, the size of thread/mod.rs is just getting out of hand.

std::thread::thread here we come?

EDIT: I was right!

ChrisDenton
ChrisDenton reviewed Nov 5, 2024
View reviewed changes
Copy link
Member

@ChrisDenton ChrisDenton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd be happy with this approach but it would be nice to have some consensus on it to (hopefully) avoid further churn. I won't insist on it though.

library/std/src/thread/mod.rs Outdated
Comment on lines 1214 to 1210
#[cfg(not(target_thread_local))]
#[allow(unused)]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this change is needed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Because this is also used when 64-bit atomics are unavailable.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any more specific cfg that can be used? #[allow(unused)], while sometimes necessary, makes me nervous because it could silently become actual dead code in the future.

library/std/src/thread/thread.rs Outdated
Comment on lines 13 to 14
// INVARIANT: must be valid UTF-8
name: Option<CString>,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change seems worse. Having an invariant that can be violated in safe code is not great.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also wondered if it made sense to have a new-type that held both invariants (UTF-8 & c-string \0 ending) ?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Utf8CStr is a fairly common type in the ecosystem, it would make sense for us to start tinkering with providing it.

@ChrisDenton
Copy link
Member

Also could you separate the file move into a different commit? It's easier to diff that way.

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@joboet
Copy link
Member Author

joboet commented Nov 7, 2024

Blocked on the efficiency fix in #132730, without that, we'd be creating a new Thread for every blocking RwLock call on macOS.

@rustbot label +S-blocked

@rustbot rustbot added the S-blocked Status: Blocked on something else such as an RFC or other implementation work. label Nov 7, 2024
@joboet joboet added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 7, 2024
@joboet joboet linked an issue Nov 12, 2024 that may be closed by this pull request
std::thread::Thread grew from one to two pointer sizes #132619
Closed
@bors
Copy link
Collaborator

bors commented Nov 20, 2024

☔ The latest upstream changes (presumably #133219) made this pull request unmergeable. Please resolve the merge conflicts.

@rustbot rustbot added O-unix Operating system: Unix-like O-windows Operating system: Windows labels Nov 25, 2024
@joboet
Copy link
Member Author

joboet commented Nov 25, 2024

I've undone the Thread move (that's probably worth another PR) and fixed some other things related to signal handling (thread::current now allocates on the main thread, so we mustn't call it in the signal handler).

@rustbot ready

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Nov 25, 2024
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors
Copy link
Collaborator

bors commented Jan 9, 2025

☔ The latest upstream changes (presumably #135279) made this pull request unmergeable. Please resolve the merge conflicts.

@bors
Copy link
Collaborator

bors commented Jan 11, 2025

☔ The latest upstream changes (presumably #135357) made this pull request unmergeable. Please resolve the merge conflicts.

joboet added 4 commits January 14, 2025 13:37
@joboet
Revert "Remove the Arc rt::init allocation for thread info"
0e5ee89 
This reverts commit 0747f28.
@joboet
std: lazily allocate the main thread handle
14f7f4b 
Thereby, we also allow accessing thread::current before main: as the runtime no longer tries to install its own handle, this will no longer trigger an abort. Rather, the name returned from name will only be "main" after the runtime initialization code has run, but I think that is acceptable.

This new approach also requires some changes to the signal handling code, as calling `thread::current` would now allocate when called on the main thread, which is not acceptable. I fixed this by adding a new function (`with_current_name`) that performs all the naming logic without allocation or without initializing the thread ID (which could allocate on some platforms).
@joboet
make sure that the allocator is actually called in allocator test
1d25252 
Originally authored by GnomedDev
@joboet
add comments explaining main thread identification
2c28cc4
@workingjubilee
Copy link
Member

@bors r=ChrisDenton

@bors
Copy link
Collaborator

bors commented Jan 14, 2025

📌 Commit 2c28cc4 has been approved by ChrisDenton

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2025
This was referenced Jan 15, 2025
Closed
Merged
@bors bors merged commit 4c26dc5 into rust-lang:master Jan 15, 2025
6 checks passed
@rustbot rustbot added this to the 1.86.0 milestone Jan 15, 2025
@joboet joboet mentioned this pull request Jan 17, 2025
Open
@faern faern mentioned this pull request Jan 31, 2025
Merged
@nablaa nablaa mentioned this pull request Jun 1, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fbstj fbstj fbstj left review comments

@RalfJung RalfJung RalfJung left review comments

@workingjubilee workingjubilee workingjubilee left review comments

@ChrisDenton ChrisDenton ChrisDenton approved these changes

Assignees

@ChrisDenton ChrisDenton

Labels
O-unix Operating system: Unix-like O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.86.0
Development

Successfully merging this pull request may close these issues.

std::thread::Thread grew from one to two pointer sizes
8 participants
@joboet @rustbot @rust-log-analyzer @workingjubilee @ChrisDenton @bors @fbstj @RalfJung